Don't fetch block numbers nor check rpc version for ignored tests #3742

Abeeujah · 2025-09-22T10:56:09Z

Introduced changes

Block number and RPC version for ignored tests is no longer being checked

fork_config returns None for ignored tests
should_be_run IgnoredFilter::Ignored match arm asserts that test case is ignored.

Checklist

cptartur · 2025-09-24T08:44:41Z

crates/forge/src/test_filter.rs

+            IgnoredFilter::Ignored => {
+                assert!(ignored);
+                true
+            }


Don't think we should be including this change, it's technically true given our current logic, but I think it's safer to still check if the test is ignored here since it's not very expensive.

cptartur · 2025-09-24T08:54:59Z

crates/forge/src/run_tests/resolve_config.rs

+                fork_config: if case.config.ignored {
+                    None
+                } else {
+                    resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
+                        .await?
+                },


This isn't correct, in case forge is run with --ignored or --include-ignored, they will not have their configs resolved.

We should probably split separate the steps test_target_with_config and resolve_config and call them as separate methods, not as a part of test_package_with_config_resolved.

Then we can pass TestsFilter to resolve_config and call resolve_fork_config based on if should_be_run is true for a test case.

We could probably make do without splitting from test_package_with_config_resolved and just passing TestFilter to the method, but it seems a bit weird to me to use test filter without we even did any kind of filtering with it.

Passing tests_filter to test_package_with_config_resolved would require marking the ignored_filter field pub, that looks like a change that would be needing approval?

Filtering is being done there, tests which the fork config should be resolved or not is going to be dependent on the tests_filter argument being passed there, if it wasn't for that, then we'll be running into the issue described above, unless there's another way to resolve that.

cptartur · 2025-09-24T17:20:48Z

crates/forge/src/run_tests/resolve_config.rs

+                fork_config: if ignored && matches!(ignored_filter, IgnoredFilter::NotIgnored) {
+                    None
+                } else {
+                    resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
+                        .await?
+                },


We are duplicating the filtering logic here, why not use TestsFilter.should_be_run instead?
If the test isn't going to be run for whatever reason (including being ignored), then we don't need to resolve fork config for it.

I tried using TestsFilter.should_be_run but it expects test_case: &TestCaseWithResolvedConfig while in this scenario, the test case being filtered is TestCase<TestCaseConfig> that's why I did that, also worth mentioning that TestsFilter.should_be_run would require me sharing the whole TestsFilter instance instead of just the needed ignored_filter field.

I tried using TestsFilter.should_be_run but it expects test_case: &TestCaseWithResolvedConfig while in this scenario, the test case being filtered is TestCase

What could be done in that case is we should introduce a trait that both TestCaseConfig and TestCaseResolvedConfig that contains one method like is_ignored. Then should_be_run and relevant TestCaseFilter can be made generic, so they take TestCase<T> where T implements the introduced trait.

This way it can be reused between both test_case types and logic can be shared.

also worth mentioning that TestsFilter.should_be_run would require me sharing the whole TestsFilter instance instead of just the needed ignored_filter field.

Passing a reference to it to the function doesn't incur any significant cost

Hi @cptartur I did implement this, and requested a review from you, looking forward to getting it reviewed at your earliest convenience

cptartur

We should also add some tests checking if fork configs get properly resolved (or not resolved) based on the ignored filter.

cptartur · 2025-09-29T19:46:12Z

crates/forge/src/run_tests/resolve_config.rs

+                fork_config: if ignored && tests_filter.should_be_run(&case) {
+                    None
+                } else {
+                    resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
+                        .await?
+                },


I think the condition should be flipped. Also, just checking for if the tests should be run should suffice, we don't need to also check if it was ignored.

Suggested change

fork_config: if ignored && tests_filter.should_be_run(&case) {

None

} else {

resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)

.await?

},

fork_config: tests_filter.should_be_run(&case) {

resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)

.await?

} else {

None

},

Flipped, thanks for mentioning, this sounds more intuitive actually.

cptartur · 2025-09-29T19:47:32Z

crates/forge/src/test_filter.rs

+impl Default for TestsFilter {
+    fn default() -> Self {
+        Self {
+            name_filter: NameFilter::All,
+            ignored_filter: IgnoredFilter::All,
+            last_failed_filter: false,
+            skip_filter: Vec::new(),
+            failed_tests_cache: FailedTestsCache::default(),
+        }
+    }
+}


If the default is just for tests, let's implement it behind #[cfg(tests)]. I don't think it's a good idea to have a default for this kind of structure.

I've put it behind #[cfg(test)]

I think it will be easier to just create this in tests directly, as I wrote in the comment above.

cptartur

I see a bunch of tests is failing, not sure if due to changes from this PR. Maybe try updating from master

cptartur · 2025-09-30T21:01:39Z

crates/forge/src/run_tests/resolve_config.rs

+        let ignored =
+            case.config.ignored || (env_ignore_fork_tests && case.config.fork_config.is_some());


This could be inlined again

cptartur · 2025-09-30T21:02:21Z

crates/forge/src/run_tests/resolve_config.rs

+        let ignored =
+            case.config.ignored || (env_ignore_fork_tests && case.config.fork_config.is_some());
+        test_cases.push(TestCaseWithResolvedConfig {
+            config: TestCaseResolvedConfig {
+                available_gas: case.config.available_gas,
+                ignored,
+                fork_config: if tests_filter.should_be_run(&case) {
+                    resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
+                        .await?
+                } else {
+                    None
+                },
+                expected_result: case.config.expected_result,
+                fuzzer_config: case.config.fuzzer_config,
+                disable_predeployed_contracts: case.config.disable_predeployed_contracts,
+            },
+            name: case.name,
+            test_details: case.test_details,
+        });


Let's try not modify code that is not relevant to the PR changes

This right here was due to Rust reporting a partial move, the name, test_details, expected_result, fuzzer_config, disable_predeployed_contracts don't implement copy, so they get partially moved, before I try to borrow &case in should_be_run. This is why I modified it to be that way.

cptartur · 2025-09-30T21:11:50Z

crates/forge/src/run_tests/resolve_config.rs

+            BlockId::BlockNumber(100),
+        )];
+
+        let tests_filter = TestsFilter::default();


I think it would be clearer to create one directly here otherwise you have to know the implementation of default to know if this test makes sense.

cptartur · 2025-09-30T21:44:02Z

crates/forge/src/test_filter.rs

+impl Default for TestsFilter {
+    fn default() -> Self {
+        Self {
+            name_filter: NameFilter::All,
+            ignored_filter: IgnoredFilter::All,
+            last_failed_filter: false,
+            skip_filter: Vec::new(),
+            failed_tests_cache: FailedTestsCache::default(),
+        }
+    }
+}


I think it will be easier to just create this in tests directly, as I wrote in the comment above.

Abeeujah · 2025-10-01T17:07:54Z

The failing tests seem to look like they all expect std contains some text... assert_stdout_contains

dev: Don't fetch block numbers nor check rpc version for ignored tests

824de94

Abeeujah requested a review from a team as a code owner September 22, 2025 10:56

Abeeujah requested review from ddoktorski and MKowalski8 September 22, 2025 10:56

Merge remote-tracking branch 'origin/master' into ignore-flag

5fd44ff

cptartur requested changes Sep 24, 2025

View reviewed changes

Abeeujah added 4 commits September 24, 2025 14:00

dev: Revert to inexpensive safety check

bb4e267

dev: check tests filter before resolving fork config

8546e3e

Merge branch 'master' into ignore-flag

d09df8f

dev: Fix Visibility and Ownership

dee6faf

cptartur requested changes Sep 24, 2025

View reviewed changes

Abeeujah added 2 commits September 26, 2025 17:30

dev: Introduce TestCaseIsIgnored trait

c0eeffb

dev: Implement Default Trait for TestsFilter

5b466f8

Abeeujah requested a review from cptartur September 26, 2025 16:48

cptartur reviewed Sep 29, 2025

View reviewed changes

Abeeujah added 2 commits September 30, 2025 11:53

dev: Test Fork Config Resolution

a6cba94

dev: Update branch

11463a7

Abeeujah requested a review from cptartur September 30, 2025 11:12

cptartur reviewed Sep 30, 2025

View reviewed changes

Abeeujah and others added 2 commits October 1, 2025 09:12

Merge branch 'master' into ignore-flag

a6d2fdc

dev: Remove Default impl for TestsFilter

0da75a2

Abeeujah requested a review from cptartur October 2, 2025 12:50

		let ignored =
		case.config.ignored \|\| (env_ignore_fork_tests && case.config.fork_config.is_some());

Don't fetch block numbers nor check rpc version for ignored tests #3742

Are you sure you want to change the base?

Don't fetch block numbers nor check rpc version for ignored tests #3742

Conversation

Abeeujah commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduced changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cptartur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cptartur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abeeujah commented Oct 1, 2025

Uh oh!

Uh oh!

Abeeujah commented Sep 22, 2025 •

edited

Loading